revert "feat: added soft delete functionality"#13
Conversation
This reverts commit 9dc3367.
There was a problem hiding this comment.
Pull request overview
This PR reverts the soft delete functionality that was added in PR #6, removing the ability to soft-delete and restore discussion threads, comments, and responses.
Key Changes:
- Removed soft delete/restore UI elements, API endpoints, and related state management
- Reverted filtering capabilities for viewing deleted vs. active content
- Removed "Deleted by" banners and visual indicators for deleted content
Reviewed changes
Copilot reviewed 46 out of 47 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/index.scss | Removed CSS styling for deleted content indicators, learner badges, and submenu containers |
| src/discussions/utils.js | Removed restore action and simplified action permission checking |
| src/discussions/posts/post/messages.js | Removed message definitions for deleted content badges and restore dialogs |
| src/discussions/posts/post/PostLink.jsx | Removed deleted content visual indicators and threading logic for responses/comments |
| src/discussions/posts/post/Post.jsx | Removed restore confirmation dialog and deleted-by banner display |
| src/discussions/posts/post-filter-bar/messages.js | Removed filter messages for active/deleted content |
| src/discussions/posts/index.js | Removed PostLink export from main posts module (still exported from post/index.js) |
| src/discussions/posts/data/thunks.js | Removed restore thread functionality and deleted content API filtering |
| src/discussions/posts/data/slices.js | Reverted default status filter from ACTIVE to ALL and removed deleted view toggle |
| src/discussions/posts/data/selectors.js | Removed deleted view selector |
| src/discussions/posts/data/api.js | Removed restoreThread API function and isDeleted parameter |
| src/discussions/posts/data/factories/threads.factory.js | Removed is_deleted field from thread factory |
| src/discussions/posts/PostsView.test.jsx | Updated test assertions to reflect removed "active" filter terminology |
| src/discussions/posts/NoResults.jsx | Modified learners filter null check logic |
| src/discussions/post-comments/messages.js | Removed restore and deleted content message definitions for comments |
| src/discussions/post-comments/data/thunks.js | Removed restore comment functionality and showDeleted parameter |
| src/discussions/post-comments/data/hooks.js | Removed deleted content visibility logic |
| src/discussions/post-comments/data/api.js | Removed restoreComment API and showDeleted parameters |
| src/discussions/post-comments/data/factories/comments.factory.js | Removed is_deleted field from comment factory |
| src/discussions/post-comments/comments/comment/Reply.jsx | Removed restore functionality and deleted-by banner |
| src/discussions/post-comments/comments/comment/CommentHeader.jsx | Removed flex-wrap from header styling |
| src/discussions/post-comments/comments/comment/Comment.jsx | Removed restore dialog, deleted content handling, and related state |
| src/discussions/post-comments/PostCommentsView.test.jsx | Removed show_deleted from test mocks |
| src/discussions/messages.js | Removed restore action and bulk operation messages |
| src/discussions/learners/utils.js | Removed restore actions from learner action list and submenu utility |
| src/discussions/learners/messages.js | Removed deleted activity and restore-related message definitions |
| src/discussions/learners/learner/proptypes.js | Removed deleted content count properties |
| src/discussions/learners/learner/LearnerFooter.jsx | Removed deleted activity statistics display |
| src/discussions/learners/learner/LearnerFilterBar.jsx | Removed deleted activity filter option |
| src/discussions/learners/learner/LearnerCard.jsx | Removed deleted count props |
| src/discussions/learners/learner-post-filter-bar/LearnerPostFilterBar.test.jsx | Updated test expectations for reduced filter count |
| src/discussions/learners/learner-post-filter-bar/LearnerPostFilterBar.jsx | Removed content status filter and simplified filter state management |
| src/discussions/learners/data/thunks.js | Removed restore user posts functionality and deleted content endpoint usage |
| src/discussions/learners/data/slices.js | Removed contentStatus from post filter and undelete-related actions |
| src/discussions/learners/data/redux.test.jsx | Updated test to match simplified filter structure |
| src/discussions/learners/data/api.js | Removed restore and deleted content API functions |
| src/discussions/learners/LearnerPostsView.test.jsx | Removed submenu hover interactions from tests |
| src/discussions/learners/LearnerPostsView.jsx | Removed restore confirmation dialog and simplified action handlers |
| src/discussions/learners/LearnerActionsDropdown.test.jsx | Removed submenu-related test interactions |
| src/discussions/learners/LearnerActionsDropdown.jsx | Simplified dropdown to remove submenu functionality |
| src/discussions/data/selectors.js | Updated filter check to remove ACTIVE status check |
| src/discussions/data/constants.js | Removed THREAD_FILTER_TYPES constants |
| src/discussions/common/HoverCard.jsx | Removed disabled state for deleted content |
| src/discussions/common/ActionsDropdown.jsx | Removed disabled action handling |
| src/data/constants.js | Removed RESTORE, RESTORE_COURSE_POSTS, RESTORE_ORG_POSTS actions and ACTIVE/DELETED filters |
| src/components/FilterBar.jsx | Removed active/deleted filter options and separator logic |
| src/assets/undelete.svg | Removed restore icon SVG file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| white-space: normal; | ||
| text-align: left; | ||
| border: none !important; | ||
| min-width: 195px !important |
There was a problem hiding this comment.
Missing semicolon at the end of the CSS property declaration. This line should end with a semicolon to follow CSS syntax rules.
| const learnersFilter = useSelector(({ learners }) => learners.usernameSearch); | ||
| const isFiltered = postsFiltered || (topicsFilter !== '') | ||
| || (learnersFilter) || (inContextTopicsFilter !== ''); | ||
| || (learnersFilter !== null) || (inContextTopicsFilter !== ''); |
There was a problem hiding this comment.
The comparison logic is incorrect. The condition should check if learnersFilter is not null/empty, but the original code was checking learnersFilter directly (truthy check). The current change learnersFilter !== null will still evaluate to true when learnersFilter is an empty string, which may not be the intended behavior. Consider using a proper empty/falsy check instead.
| navigate({ ...discussionsPath(Routes.LEARNERS.PATH, { courseId })(location) }); | ||
| } | ||
| }, [courseId, username, hideRestoreConfirmation, dispatch, loadMorePosts, postId, navigate, location]); | ||
| }, [courseId, username, hideDeleteConfirmation]); |
There was a problem hiding this comment.
The handleDeletePosts callback is missing dependencies in its dependency array. The callback uses dispatchDelete, navigate, and location but only includes courseId, username, and hideDeleteConfirmation. This could lead to stale closures and unexpected behavior. Add dispatchDelete, navigate, and location to the dependency array.
| }, [courseId, username, hideDeleteConfirmation]); | |
| }, [courseId, username, hideDeleteConfirmation, dispatchDelete, navigate, location]); |
| // For delete action we check `content.canDelete` | ||
| if (action === ContentActions.DELETE) { | ||
| return true; |
There was a problem hiding this comment.
The checkPermissions function now always returns true for DELETE action, which bypasses the actual permission check. This should return content.canDelete instead to properly validate deletion permissions.
| })} | ||
| > | ||
| <div className="align-items-center d-flex flex-row flex-wrap"> | ||
| <div className="align-items-center d-flex flex-row"> |
There was a problem hiding this comment.
The flex-wrap style was removed from this div element, which may cause layout issues when the content overflows. If the child elements need to wrap to the next line (e.g., author labels, timestamps), removing flex-wrap could cause them to overflow their container instead of wrapping properly.
| <div className="align-items-center d-flex flex-row"> | |
| <div className="align-items-center d-flex flex-row flex-wrap"> |
| onClick={handleResponseCommentButton} | ||
| disabled={isClosed || isDeleted} | ||
| style={{ lineHeight: '20px', ...(isDeleted ? { opacity: 0.3, cursor: 'not-allowed' } : {}) }} | ||
| onClick={() => handleResponseCommentButton()} |
There was a problem hiding this comment.
The button's onClick handler is unnecessarily wrapped in an arrow function. The current code onClick={() => handleResponseCommentButton()} should be simplified to onClick={handleResponseCommentButton} for better performance and cleaner code, unless parameters need to be passed.
| onClick={() => handleResponseCommentButton()} | |
| onClick={handleResponseCommentButton} |
| className="icon-size-24" | ||
| /> | ||
| <span className="font-weight-normal ml-2"> | ||
| {action.label.defaultMessage} |
There was a problem hiding this comment.
The message formatting with intl.formatMessage should be used here instead of directly accessing action.label.defaultMessage. This bypasses the internationalization system and will not respect the user's language preferences or locale settings.
| {action.label.defaultMessage} | |
| {intl.formatMessage(action.label)} |
| showDeleteConfirmation(); | ||
| await dispatch(deleteUserPosts(courseId, username, courseOrOrg, false)); | ||
| }, [courseId, username, showDeleteConfirmation, dispatch]); | ||
| }, [courseId, username, showDeleteConfirmation]); |
There was a problem hiding this comment.
The handleShowDeleteConfirmation callback is missing dispatch in its dependency array. The callback uses dispatch but only includes courseId, username, and showDeleteConfirmation. This could lead to stale closures. Add dispatch to the dependency array.
| }, [courseId, username, showDeleteConfirmation]); | |
| }, [courseId, username, showDeleteConfirmation, dispatch]); |
| const handleActions = useCallback((action) => { | ||
| const actionFunction = actionHandlers[action]; | ||
| if (actionFunction) { | ||
| actionFunction(); | ||
| close(); | ||
| } | ||
| }, [actionHandlers, close]); | ||
| }, [actionHandlers]); |
There was a problem hiding this comment.
The handleActions callback is missing close in its dependency array. While close is called in the onClick handler on line 73, it should be included in the handleActions dependencies for consistency, or the close() call should be moved to the onClick handler instead of inside handleActions to avoid confusion.
Reverts #6